Explain how to temporarily use paths instead of diagnostic items #15150
Conversation
rustbot
added
the
S-waiting-on-review
samueltardieu
force-pushed
the
stdlib-diag-items-test
branch
2 times, most recently
from
e574d7c to
b26ac4d
Compare
Paths into the standard library may be added temporarily until a diagnostic item is added to the library and synced with Clippy sources. This documents how to do this. This also adds a test to check that consistent and easy to notice names are used, and that a PR into `rust-lang/rust` has been opened to add the corresponding diagnostic items. The test is a no-op if run as part of the compiler test suite and will always succeed.
samueltardieu
force-pushed
the
stdlib-diag-items-test
branch
from
b26ac4d to
f1bc376
Compare
| // This tests checks that if a path is defined for an entity in the standard | ||
| // library, the proper prefix is used and a reference to a PR against | ||
| // `rust-lang/rust` is mentionned. |
There was a problem hiding this comment.
I'm personally not a fan of the idea of enforcing that a PR must be made to the r-l/r repository with a test like that. It's a rather huge hit in ergonomics and I imagine increases the barrier of entry for new contributors, rust-lang/rust is a huge project that takes a bit to set up, build the tools and maybe sometimes bless tests.
I do know that we generally prefer diagnostic items over hardcoded paths whenever possible, but when that isn't possible IMO the way it used to be done asynchronously (i.e., people add paths and every once in a while someone goes through them and adds diagnostic items in bulk) worked fine.
The argument that diagnostic items are more resilient against refactors in the stdlib is true but IMHO still isn't strong enough to warrant this. It's not like moving an item in the stdlib will silently break clippy, since clippy is built and tested in CI, so they'll just have to change the paths.rs file in clippy (which they might do anyway if they grep for the path across the codebase).
Then again, it's possible I'm alone on this (I know of #5393 and that it's taken a while to get to closing it). Might be something worth discussing in a meeting. I could be convinced that it's something we should do if more people think it's a good idea, or with more motivation
| // | ||
| // During development, the "Added in …" comment is not required, but will make the | ||
| // test fail once the PR is submitted and becomes mandatory to ensure that a proper | ||
| // PR against `rust-lang/rust` has been created. |
There was a problem hiding this comment.
One thing that's unclear to me is what happens if a Clippy PR that defines one such path is ready to be merged but the nightly hasn't been bumped yet so we don't have the diagnostic item.
Would that be considered a blocker for the Clippy PR and the PR author will need to wait for that bump to happen, and then remove the path here?
Or can the clippy PR be merged with the temporary path in this file? If this is the case, who migrates it to a diagnostic item? It seems like we would essentially be back to the current situation of people having to go through the paths every once in a while asynchronously anyway independent of the PR and the test wouldn't actually enforce the use of diagnostic items, would it?
Paths into the standard library may be added temporarily until a diagnostic item is added to the library and synced with Clippy sources. This documents how to do this.
This also adds a test to check that consistent and easy to notice names are used, and that a PR into
rust-lang/rusthas been opened to add the corresponding diagnostic items.The test is a no-op if run as part of the compiler test suite and will always succeed.
changelog: none
r? @Alexendoo